Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for download share on old android browser #17623

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

j3l11234
Copy link
Contributor

In old android browser, The browser can't follow the cookies, but the browser's downloader can't follow cookies. So I skip the sameSite check when download share

__Host-nc_sameSiteCookielax=true;
__Host-nc_sameSiteCookiestrict=true

Signed-off-by: j3l11234 <297259024@qq.com>
@kesselb
Copy link
Contributor

kesselb commented Oct 21, 2019

Thanks for this contribution 👍

In old android browser,

Could you elaborate? How can I test this change?

@j3l11234
Copy link
Contributor Author

Thanks for this contribution 👍

In old android browser,

Could you elaborate? How can I test this change?

OK,My Phone is OPPO R9m / Android 5.1 , I use the browser that comes with the system. I use proxy to monitor network
It can show the share page. The request has the sameSite cookies.
But when I click the download, It show the download panel, but cant start the download. I see many many 302 with the set-cookies. The browser's downloader just follow the 302, but not follow the set-cookies of sameSite.

@kesselb kesselb added 3. to review Waiting for reviews bug enhancement and removed bug labels Oct 21, 2019
@kesselb kesselb requested a review from rullzer October 21, 2019 14:07
@kesselb kesselb added this to the Nextcloud 18 milestone Oct 21, 2019
@rullzer
Copy link
Member

rullzer commented Oct 21, 2019

I need to think about this.
But I'm not a fan of dropping security related things for an obscure old android browser.

@j3l11234
Copy link
Contributor Author

I need to think about this.
But I'm not a fan of dropping security related things for an obscure old android browser.

In fact is not old android browser. In my Android 10, many browser app has the same problem. such as UC
I think when download the public share, who has the share key should can download freely. ( In publish share page, should not rely on cookies )

@rullzer
Copy link
Member

rullzer commented Oct 26, 2019

Only partly true of course. With password protected public links we should rely on cookies.
In that case we should for sure require the samesite cookies.

I'll dive into this a bit later

@j3l11234
Copy link
Contributor Author

j3l11234 commented Nov 6, 2019

Only partly true of course. With password protected public links we should rely on cookies.
In that case we should for sure require the samesite cookies.

I'll dive into this a bit later

The purpose of 'nc_sameSiteCookie*' is want to protect logout CSRF. I think the checking (check 'nc_sameSiteCookie*' and set it if it doesn't exist. the module is SameSiteCookieMiddleware) should not be apply to the 'downloadShare' func.
Actually when visit the share page, the 'nc_sameSiteCookie*' must has been set.
Actually when we always set the cookies with SameSite.

@rullzer rullzer mentioned this pull request Dec 11, 2019
43 tasks
@j3l11234
Copy link
Contributor Author

@rullzer What's going on?

This was referenced Dec 12, 2019
This was referenced Dec 27, 2019
@rullzer
Copy link
Member

rullzer commented Jan 7, 2020

So I took another look at this, your issues seems valid if you use any kind of download manager indeed.

However to fully merge this and thus ignore the samesitecookie. I'l like to make sure this uses full appframework code. So that we have real file download responses with all the security checks that that adds. I'll see if I can fix that early next week.

@rullzer rullzer self-assigned this Jan 7, 2020
@rullzer rullzer modified the milestones: Nextcloud 18, Nextcloud 19 Jan 7, 2020
This was referenced Apr 4, 2020
This was referenced Apr 15, 2020
@rullzer rullzer mentioned this pull request Apr 23, 2020
11 tasks
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

@rullzer rullzer merged commit 4114bce into nextcloud:master Apr 24, 2020
@welcome
Copy link

welcome bot commented Apr 24, 2020

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants